Skip to content

fix(azure): preserve membership consistency - #11309

Merged
ReubenBond merged 16 commits into
dotnet:mainfrom
ReubenBond:rb-fix-azure-membership-consistency
Sep 18, 2026
Merged

ReubenBond merged 16 commits into
dotnet:mainfrom
ReubenBond:rb-fix-azure-membership-consistency

Conversation

@ReubenBond

@ReubenBond ReubenBond commented Sep 17, 2026 •

Copy link
Copy Markdown
Member

Problem

Heartbeat-only writes change physical Azure row etags, creating false contention for membership updates. Cleanup currently selects entries using Azure's storage timestamp rather than membership status and latest known activity.

Solution

  • Reuse the ordered !Start and ~End markers already written atomically with membership changes for paginated ReadAll queries. Matching marker/canonical versions establish consistency; mismatches retry within the existing bound and missing markers fail explicitly. ReadRow retains its existing single filtered query for the requested member and canonical version row.
  • Return each silo row's actual Azure etag. Protect canonical membership updates with the authoritative VersionRow etag CAS in the same atomic transaction as a wildcard existing-row replacement and the boundary-marker updates. This preserves stale-version and missing-row atomic failure while allowing updates to proceed across heartbeat-only changes to the physical row etag.
  • Publish each heartbeat with a single unconditional merge of the IAmAlive column, relying on the owning silo as its sole writer. The merge directly propagates native storage errors and cancellation. IAmAlive follows best-effort semantics independently of canonical membership concurrency.
  • Propagate storage, authorization, and network failures, including table-not-found responses and mixed cleanup batch failures. The internal shared-source TaskUtilities.WhenAllWithAggregateException helper awaits every task, propagates single faults and cancellation normally, and uses ExceptionDispatchInfo.Throw to expose multiple failures as an aggregate. Azure cleanup compiles the shared source through a project link; the helper remains internal in each consuming assembly.
  • Prune only Dead entries whose latest start, heartbeat, and suspicion timestamps precede the exclusive cutoff. Each cleanup invocation selects once and awaits its conditional deletion batches, preserving every batch failure for the caller. Native row etags protect eligibility races; the existing cleanup agent schedules later attempts. Cleanup retains the existing bulk limits and preserves the membership version, consistent with fix(membership): preserve canonical membership views #11296.
  • Add focused native Azure SDK regressions for single-query reads, marker validation across ReadAll pages, updates using the original version condition after heartbeats, atomic conditional writes, the single-request heartbeat path, cutoff precision, cancellation, detached results, culture-invariant opaque etags, and infrastructure errors. Shared-helper tests cover await-all completion, fault identity/stack preservation, aggregates, cancellation precedence, and synchronous input validation.

The first commit is the shared legacy cleanup-test prerequisite from #11308: source bede893d290a9035f196f4c3ed58213d102efc6c, cherry-picked here as 8633ec7a62d83832c8cddb70f0f188848e4abd44. Subsequent commits contain the Azure provider changes and focused regressions, plus the requested internal common task-helper extraction. The generated public API surface matches main.

Microsoft Reviewers: Open in CodeFlow

Copilot AI lite review requested due to automatic review settings September 17, 2026 22:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Reuse the fenced cleanup snapshot across successful batches to avoid repeated full scans.

Review effort: Lite
Findings: None

What changed in this PR

Strengthens Azure membership consistency under concurrent reads, liveness updates, and cleanup.

Changes:

  • Adds ETag-fenced membership reads and conditional liveness writes.
  • Restricts cleanup to stale Dead entries.
  • Expands Azure SDK regression coverage and shared cleanup tests.
File Description
test/​Orleans.Runtime.Internal.Tests/​MembershipTests/​MembershipTableTestsBase.cs Updates shared cleanup expectations.
test/​Extensions/​Orleans.Azure.Tests/​AzureMembershipPaginationTests.cs Adds read, write, cleanup, cancellation, and error regressions.
src/​Azure/​Orleans.Clustering.AzureStorage/​OrleansSiloInstanceManager.cs Implements fencing, conditional writes, and recency-based cleanup; cleanup rescans the full partition for each successful batch, causing unnecessary O(N²/B) read traffic.
src/​Azure/​Orleans.Clustering.AzureStorage/​AzureBasedMembershipTable.cs Surfaces malformed membership rows.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Code coverage

Metric Pull request
Lines 82.43% (112,984 / 137,074)
Branches 71.70% (32,675 / 45,569)

Report-only conclusion: current-main baseline stale.

The newest successful coverage run tested 5267011, not current main 53b0307.

Coverage combines every CI test matrix job, including providers, CodeGen, .NET 8/10, Linux, Windows, and macOS, using canonical physical source and branch identities.

The comparison remains report-only while normal line and branch variance is calibrated.

Coverage details

Copilot AI review requested due to automatic review settings September 17, 2026 23:07
@ReubenBond

Copy link
Copy Markdown
Member Author

Addressed the cleanup review in e4458f6. Cleanup now passes the selected rows to the existing DeleteEntriesBatch helper, reusing one fenced snapshot across all successful batches. Row-etag contention triggers a fresh selection after outstanding batches complete. Conditional deletions continue to protect concurrent heartbeats and votes.

The regression checks exactly one partition query and two version reads for 195 deletions with batch limits 1, 2, 3, and 100. The contention case now covers a successful batch followed by a conflicting batch, then confirms that refreshed liveness prevents deletion on retry.

I also consolidated repeated heartbeat-failure setup and empty/ineligible-cleanup cases, and reused the existing SDK mock factory. The PR is 57 net lines smaller while retaining all 76 focused cases. The net10.0 build and focused suite pass.

The small shared cleanup-test prerequisite remains necessary until #11308 lands; rebasing afterward will remove that fourth file from this PR's diff.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Cleanup validation and failure-propagation issues remain unresolved.

Review effort: Lite
Findings: None

Copilot AI review requested due to automatic review settings September 18, 2026 15:11
@ReubenBond

Copy link
Copy Markdown
Member Author

Follow-up to #11309 (review): the summary had no inline details, but the failure-handling audit identified and reproduced two concrete gaps. Fixed in 38cf2de:

  • Concurrent cleanup now preserves every failed batch in an AggregateException and retries only when every failure is row contention. Mixed 412 + authorization/service failures propagate, regardless of batch order. Successful batches still reuse the same fenced snapshot.
  • Conditional updates use GetEntityAsync, letting the real SDK distinguish missing rows (conditional failure) from TableNotFound (propagated infrastructure failure).

Regressions: CleanupPropagatesMixedBatchFailures, CleanupRetriesWhenEveryBatchFailureIsContention, and UpdateDistinguishesMissingRowFromMissingTableThroughSdk. The latter uses the actual TableClient with an in-memory HTTP transport, exercising SDK 404 classification. Existing tests continue to cover valid cleanup batch limits 1/2/3/100, recency/cutoff equality, cancellation, and row-etag safety; the review summary supplied no additional concrete validation requirement.

The new regressions reproduced five failures before the fix. The revised focused net10.0 class passes 84/84, with a clean build. Both Azure Storage CI jobs must pass on this exact head before this follow-up is considered complete. The PR remains draft.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The membership consistency and concurrency changes require final human review.

Review effort: Lite
Findings: None

@ReubenBond

Copy link
Copy Markdown
Member Author

The required Azure CI gate is now satisfied for exact head 38cf2de61751b0a60fa857a529f664ffcbe500f3:

This supplements the 84/84 focused regression run and clean local build. The PR remains draft; other checks are still running.

@ReubenBond
ReubenBond marked this pull request as ready for review September 18, 2026 15:33
Copilot AI review requested due to automatic review settings September 18, 2026 15:39
@ReubenBond

Copy link
Copy Markdown
Member Author

Simplified heartbeat writes in cca608bee22edac1deef20cfb83e066dfa01d658 using the owning-silo invariant: each heartbeat is a single unconditional merge containing only IAmAlive plus entity keys. The normal path performs zero reads. A missing retired row still triggers the canonical-history check before completing, and infrastructure failures propagate.

Removed the unnecessary heartbeat read/max/CAS retry loop and its stronger multi-writer assumptions from the tests. Full-row membership updates retain their separate protection against overwriting a newer heartbeat. The patch removes 66 net lines.

OwnerHeartbeatUsesSingleUnconditionalLivenessMerge asserts one SDK call, wildcard ETag, merge mode, and the liveness-only payload. Cancellation, missing-row/history, infrastructure errors, full-row safety, and cleanup regressions remain covered. Focused net10.0 result: 80 passed, zero failed/skipped; build clean. Both Azure Storage CI jobs must succeed on this new head before validation is complete.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad Azure membership consistency and cleanup changes warrant final human review.

Review effort: Lite
Findings: None

Copilot AI review requested due to automatic review settings September 18, 2026 15:51
@ReubenBond

Copy link
Copy Markdown
Member Author

Applied the clarified owner-only heartbeat policy in ebc225082c1a8f19331201e341f49699fa8a321a: UpdateIAmAlive directly returns the one wildcard IAmAlive-only merge. All native errors, including missing-row404, propagate. Removed the canonical-history probe and the post-retirement no-op assumption.

OwnerHeartbeatUsesSingleUnconditionalLivenessMerge, HeartbeatFailuresPropagateFromSingleMerge, and HeartbeatMergeHonorsCancellation assert the one-call path on success, native errors, and cancellation. Full-row membership max/etag protection and cleanup/read consistency fixes remain unchanged.

Focused net10.0 result: 78 passed, zero failed/skipped; build: zero warnings/errors. PR description updated. Validation now follows this exact head; awaiting both Azure Storage jobs and remaining checks.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Preserve the final fence values in InconsistentStateException after retry exhaustion.

Review effort: Lite
Findings: None

Copilot AI review requested due to automatic review settings September 18, 2026 16:03
@ReubenBond

Copy link
Copy Markdown
Member Author

Applied the clarified heartbeat-independent concurrency contract in 29b652a7f460195d7e39a8be3afe8bae7a4e5c35.

ReadRow/ReadAll now expose the canonical table-version etag as the opaque membership row token. UpdateRow validates logical row/table token agreement, then issues one atomic transaction using wildcard UpdateReplace for the existing silo row and CAS for the canonical version row. Removed the row preread, heartbeat maximum comparison, and physical heartbeat-etag dependency. IAmAlive follows the accepted best-effort semantics; heartbeat publication remains one blind column merge. Raw storage etags remain internal for conditional cleanup.

OwnerHeartbeatPreservesCanonicalTokensForMembershipUpdate exercises both read APIs: an owner heartbeat changes the physical etag, the original logical tokens remain valid, an IAM-only etag change during the read fence triggers no retry, and a status/vote update using the original tokens succeeds. The test also permits the full-row update to replace the heartbeat timestamp. Stale logical-token rejection, native transaction conflict/missing-row failure, and SDK-level table failure regressions remain covered.

Focused net10.0 validation: 78 passed, zero failed/skipped; clean build. PR description reflects the latest contract. Awaiting exact-head Azure Storage net8/net10 and remaining CI checks.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in membership updates, cleanup filtering, and token-related test expectations.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)

@ReubenBond

Copy link
Copy Markdown
Member Author

Audited review #11309 (review). Its sole linked finding asks for stored-heartbeat maximum preservation, which the final agreed contract explicitly supersedes. Answered and resolved that thread with the updated #11308 contract and the native accepted-overwrite regression: #11309 (comment).

The overview's additional references to cleanup filtering and token test expectations provide no separate location or reproduction. Current focused regressions cover Dead-only/recency/cutoff filtering, conditional cleanup races, canonical-token stability across heartbeats, and true stale-token/missing-row failures. There is no additional concrete change to make from those phrases. The latest 78-case focused suite passes; exact-head CI remains the completion gate. No code change was needed for this contract clarification.

@ReubenBond

Copy link
Copy Markdown
Member Author

Final-head validation completed for 29b652a7f460195d7e39a8be3afe8bae7a4e5c35:

The heartbeat-preservation review thread is answered and resolved against the final canonical-token contract. The branch/worktree is clean; no merge was performed.

Copilot AI review requested due to automatic review settings September 18, 2026 16:59
This was referenced Oct 12, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 19, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants